Add a test case to verify GARP behavior with out of subnet IPs#23320
Add a test case to verify GARP behavior with out of subnet IPs#23320prabhataravind wants to merge 6 commits intosonic-net:masterfrom
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
github-actions
bot
requested review from
theasianpianist,
xwjiang-ms and
yutongzhang-microsoft
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
prabhataravind
force-pushed
the
paravind/garp
branch
from
3223009 to
e7acaa6
Compare
|
/azp run |
Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
|
/azp run |
|
/azpw run Azure.sonic-mgmt |
|
Retrying failed(or canceled) jobs... |
|
No Azure DevOps builds found for #23320. |
|
/azp run Azure.sonic-mgmt |
|
/azp run |
StormLiangMS
left a comment
StormLiangMS
left a comment
There was a problem hiding this comment.
@prabhataravind —
Template: ✅ OK
DCO: ✅ signed
CI: ✅ all green
Code Review:
[Important] garp_enabled fixture: == 1 → == 2 breaks existing test semantics
conftest.py:229: The garp_enabled fixture yield changed from:
yield all(int(val) == 1 for val in arp_accept_vals) # Before
yield all(int(val) == 2 for val in arp_accept_vals) # AfterThis changes what garp_enabled means for the existing test test_arp_garp_enabled:
- Before:
garp_enabled=Truemeantarp_accept=1(accept all GARPs) - After:
garp_enabled=Truemeansarp_accept=2(accept same-subnet GARPs only)
The existing test_arp_garp_enabled test sends an in-subnet GARP, so it still passes with arp_accept=2. But if any other test or downstream code relies on garp_enabled meaning "arp_accept=1", it will silently break.
Suggestion: Add a comment explaining the semantic change, or rename the fixture to clarify (e.g., garp_accept_same_subnet). Also, what kernel version introduced arp_accept=2? Older kernels only support 0/1 — document the minimum kernel requirement.
[Minor] time.sleep(2) in multiple tests — consider wait_until or retry
Tests test_arp_garp_out_of_subnet_not_learned, test_ipv6_unsolicited_na_* all use time.sleep(2) before checking the ARP table. This is brittle — if the DUT is slow processing, the test fails. For positive tests (expecting entry to exist), a wait_until with retry would be more reliable.
For negative tests (expecting entry NOT to exist), time.sleep(2) is actually reasonable — you can't wait for something that shouldn't happen. But 2 seconds may be too short on slow platforms. Consider 5 seconds to be safe.
[Minor] clear_dut_arp_cache(duthost, is_ipv6=True) — verify function signature
The is_ipv6 parameter is used in 3 new IPv6 tests. Confirm clear_dut_arp_cache actually supports this kwarg — if it doesn't, the cache clear silently does nothing and the tests could give false positives.
Overall: Good new test coverage for arp_accept=2 semantics (in-subnet, out-of-subnet, IPv6 link-local, IPv6 in/out-subnet). The fixture change is the main concern.
StormLiangMS
left a comment
StormLiangMS
left a comment
There was a problem hiding this comment.
@prabhataravind — The garp_enabled fixture yield change from == 1 to == 2 silently alters the semantics for the existing test_arp_garp_enabled test and any downstream consumers of this fixture.
Please add a comment explaining that arp_accept=2 is the new expected value (same-subnet only), and document the minimum kernel version requirement (arp_accept=2 was added in Linux 5.18+). Consider renaming the fixture if the old semantics were "accept all GARPs" vs the new "accept same-subnet only".
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Adds SONiC mgmt ARP/NDP tests to validate new GARP behavior (same-subnet-only learning) aligned with the referenced swss change that programs arp_accept=2 when grat_arp is enabled.
Changes:
- Add new extended ARP testcases for
arp_acceptvalue and out-of-subnet GARP / unsolicited NA learning behavior. - Update
garp_enabledfixture to treatarp_accept=2as the successful programmed state.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| tests/arp/test_arp_extended.py | Adds new GARP/NDP learning tests (including out-of-subnet negative cases) and a sysctl validation test. |
| tests/arp/conftest.py | Updates garp_enabled fixture to validate arp_accept==2 after enabling grat_arp. |
Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
@StormLiangMS added a comment in the fixture and also added description that this goes with a corresponding swss change. |
Description of PR
Add sonic-mgmt tests to verify GARP behavior in accordance with swss changes in sonic-net/sonic-swss#4383.
NOTE: This change is to be merged ONLY after the swss change above is merged and submodule is updated.
Summary:
Fixes # 37292328
Type of change
Back port request
Approach
What is the motivation for this PR?
To verify expected behavior with the image change to arp_accept value.
How did you do it?
By adding new test cases and changing existing tests that look for arp_accept value of 1.
How did you verify/test it?
By running the tests on physical and kvm testbeds
Any platform specific information?
N/A
Supported testbed topology if it's a new test case?
T0/dualtor
Documentation
Please refer to the details of the change and motivation in the swss PR sonic-net/sonic-swss#4383.